Skip to content

feat: move filters from superset-ui to incubator#12154

Merged
villebro merged 13 commits intoapache:masterfrom
simcha90:move-filters-ant
Jan 8, 2021
Merged

feat: move filters from superset-ui to incubator#12154
villebro merged 13 commits intoapache:masterfrom
simcha90:move-filters-ant

Conversation

@simcha90
Copy link
Contributor

@simcha90 simcha90 commented Dec 21, 2020 •
edited
Loading

SUMMARY

Move Select and Range ant filters from superset-ui to incubator + support in it for queriesData param

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

image

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@codecov-io
Copy link

codecov-io commented Dec 21, 2020 •
edited
Loading

Codecov Report

Merging #12154 (68d872d) into master (d760e88) will decrease coverage by 7.94%.
The diff coverage is 30.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12154      +/-   ##
==========================================
- Coverage   67.05%   59.11%   -7.95%     
==========================================
  Files        1001      958      -43     
  Lines       49380    46738    -2642     
  Branches     5033     4331     -702     
==========================================
- Hits        33112    27628    -5484     
- Misses      16140    19110    +2970     
+ Partials      128        0     -128
Flag Coverage Δ
cypress 51.18% <30.30%> (-0.06%) ⬇️
javascript ?
python 63.58% <ø> (-0.62%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset-frontend/src/common/components/index.tsx 100.00% <ø> (ø)
...c/dashboard/components/nativeFilters/FilterBar.tsx 48.41% <0.00%> (-4.74%) ⬇️
...rontend/src/filters/components/Range/buildQuery.ts 0.00% <0.00%> (ø)
...end/src/filters/components/Range/transformProps.ts 0.00% <0.00%> (ø)
...ontend/src/filters/components/Select/buildQuery.ts 0.00% <0.00%> (ø)
...nd/src/filters/components/Select/transformProps.ts 0.00% <0.00%> (ø)
...-frontend/src/visualizations/presets/MainPreset.js 100.00% <ø> (ø)
...set-frontend/src/filters/components/Range/index.ts 75.00% <75.00%> (ø)
...et-frontend/src/filters/components/Select/index.ts 75.00% <75.00%> (ø)
...ntend/src/filters/components/Range/controlPanel.ts 100.00% <100.00%> (ø)
... and 408 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d760e88...68d872d. Read the comment docs.

@simcha90 simcha90 closed this Dec 22, 2020
@simcha90 simcha90 reopened this Dec 22, 2020
@simcha90 simcha90 closed this Dec 22, 2020
@simcha90 simcha90 reopened this Dec 22, 2020
@simcha90 simcha90 closed this Dec 22, 2020
@simcha90 simcha90 reopened this Dec 22, 2020
@graceguo-supercat
Copy link

graceguo-supercat commented Dec 23, 2020 •
edited
Loading

Hi @simchanielsen Thanks for the PR. I think the advantage to have native filter component in superset core js codebase is obvious: quicker development cycle, easier to share function or components with other dashboard components.

But since filter component was a superset-ui plugin, may i know what is original architectural design consideration?
After moving it to superset core js, can we use/build filter component as regular dashboard component, or still need to treat native filter like other plugin? cc @villebro

@simcha90
Merge branch 'master' of github.com:apache/incubator-superset into mo…
befbde5 
…ve-filters-ant

� Conflicts:
�	superset-frontend/src/dashboard/components/nativeFilters/FilterBar.tsx
@simcha90
Merge branch 'move-filters-ant' of github.com:simchaNielsen/incubator…
7726789 
…-superset into move-filters-ant
villebro
villebro reviewed Dec 31, 2020
View reviewed changes
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for moving this over! A few first pass comments, will test soon

@junlincc junlincc added the assigned:nielsen Assigned to the nielsen team label Jan 1, 2021
villebro
villebro reviewed Jan 4, 2021
View reviewed changes
superset-frontend/src/filters/utils.ts
Comment on lines 53 to 58
if (lower !== undefined && lower !== null) {
filters.push({ col, op: '>=', val: lower });
}
if (upper !== undefined && upper !== null) {
filters.push({ col, op: '>=', val: upper });
}
Copy link
Member

@villebro villebro Jan 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The upper bound should probably be '<='

villebro
villebro approved these changes Jan 4, 2021
View reviewed changes
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for moving this + adding tests + fixing the bugs uncovered by the new tests!

@ktmud
Copy link
Member

ktmud commented Jan 8, 2021

Related: apache-superset/superset-ui#840

@villebro villebro merged commit fecfc34 into apache:master Jan 8, 2021
@villebro villebro mentioned this pull request Jan 11, 2021
Merged
6 tasks
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 First shipped in 1.0.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@villebro villebro villebro approved these changes

Assignees

No one assigned

Labels

assigned:nielsen Assigned to the nielsen team 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 First shipped in 1.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@simcha90 @codecov-io @graceguo-supercat @ktmud @villebro @mistercrunch @junlincc @amitmiran137